[Custom Transactions] Add TxBuilder trait, support fixed additional outputs
#3775
Conversation
|
👋 Thanks for assigning @carlaKC as a reviewer! |
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
c245ca1 to
7fc2092
Compare
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
7fc2092 to
6aa0848
Compare
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
fa3ea56 to
14b83a3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3775 +/- ##
==========================================
+ Coverage 89.53% 90.37% +0.84%
==========================================
Files 157 160 +3
Lines 125310 133578 +8268
Branches 125310 133578 +8268
==========================================
+ Hits 112192 120721 +8529
+ Misses 10429 10231 -198
+ Partials 2689 2626 -63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 12th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
lightning/src/ln/chan_utils.rs
Outdated
| @@ -622,6 +622,21 @@ impl HTLCOutputInCommitment { | |||
| && self.cltv_expiry == other.cltv_expiry | |||
| && self.payment_hash == other.payment_hash | |||
| } | |||
|
|
|||
| pub(crate) fn is_dust(&self, feerate_per_kw: u32, broadcaster_dust_limit_sat: u64, features: &ChannelTypeFeatures) -> bool { | |||
There was a problem hiding this comment.
If we want the interface to decide this, shouldn't it just be a new flag in HTLCOutputInCommitment rather than a method?
There was a problem hiding this comment.
I appended a commit below that makes this a flag - hopefully I've understood you :) Assuming for now we don't let users of TxBuilder decide which HTLCs are dust-vs-nondust.
| let mut value_to_self_msat_offset = 0; | ||
|
|
||
| let feerate_per_kw = feerate_per_kw.unwrap_or_else(|| self.get_commitment_feerate(funding, generated_by_local)); | ||
|
|
||
| for htlc in self.pending_inbound_htlcs.iter() { | ||
| if htlc.state.included_in_commitment(generated_by_local) { | ||
| if !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) { |
There was a problem hiding this comment.
I assume in the future this will be something the TxBuilder decides?
There was a problem hiding this comment.
Overall LN spec is moving in a direction where whether a HTLC is dust is solely a function of its amount and the dust_limit_satoshis p2p setting - so I thought we could let channel make that decision. Could we require all future HTLC-transactions (including customized ones) to pay exogenous fees ? I find them so superior so I thought this was fair.
I have an alternative branch where TxBuilder makes that decision - but it got thorny quite fast (see get_pending_htlc_stats, get_available_balances_for_scope, next_local/remote_commit_tx_fee_sat), but happy to revisit this.
There was a problem hiding this comment.
Overall LN spec is moving in a direction where whether a HTLC is dust is solely a function of its amount and the dust_limit_satoshis p2p setting - so I thought we could let channel make that decision. Could we require all future HTLC-transactions (including customized ones) to pay exogenous fees ? I find them so superior so I thought this was fair.
Hmm, but we also want to be able to sign for existing channel types, presumably, not only anchor-0FC? Also, I could see a future change to the spec removing the dust_limit_satoshis setting and just fixing it to the network level, which would also make it depend on the scriptPubKey type.
I have an alternative branch where TxBuilder makes that decision - but it got thorny quite fast (see get_pending_htlc_stats, get_available_balances_for_scope, next_local/remote_commit_tx_fee_sat), but happy to revisit this.
Yea, I imagine it will require a larger refactor such that those methods can be implemented as a function of build_commitment_stats, but sadly I just don't think its a reasonable assumption to make. Of course that doesn't mean we have to do that now, we can certainly start with that assumption and relax it later.
There was a problem hiding this comment.
Hmm, but we also want to be able to sign for existing channel types, presumably, not only anchor-0FC? Also, I could see a future change to the spec removing the dust_limit_satoshis setting and just fixing it to the network level, which would also make it depend on the scriptPubKey type.
Certainly - but currently I assume this: "only allow anchor, 0FC channels to be customized, and not static only channels."
I do see your point about removing from channel all branches on channel type. IIUC this is not a blocker for this PR let me know.
There was a problem hiding this comment.
Yea, I imagine it will require a larger refactor such that those methods can be implemented as a function of build_commitment_stats, but sadly I just don't think its a reasonable assumption to make. Of course that doesn't mean we have to do that now, we can certainly start with that assumption and relax it later.
Sounds good I'll push a refactor as a follow-up to this PR is that ok ?
There was a problem hiding this comment.
Certainly - but currently I assume this: "only allow anchor, 0FC channels to be customized, and not static only channels."
Hmm, yea, would be nice to just have a "commitment transactions are not the responsibility of channel.rs" goal. Doesn't have to get there now, but eventually would be nice to not have channel.rs have any concept for commitment transactions, only stats on them.
Yes would love to merge this as part of the 0FC PR sequence ! Also would like to merge #3855 first, to make sure whatever API we settle on here fits in nicely with a splicing world; in that world for a single set of included HTLCs based on their state, we generate multiple commitments, so this is relevant to this PR here. |
Oh! Okay, I thought we were gonna do 0FC first, but happy to do this first. |
|
What's left to get this out of draft, then? Want to make quick progress on 0FC. |
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
14b83a3 to
2541f45
Compare
tankyleo
force-pushed
the
fixed-addl-outputs
branch
3 times, most recently
from
9dba3c2 to
2f19710
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
0071eff to
5d6baeb
Compare
lightning/src/ln/channel.rs
Outdated
| if match update_state { | ||
| // Note that these match the inclusion criteria when scanning | ||
| // pending_inbound_htlcs below. | ||
| FeeUpdateState::RemoteAnnounced => { |
There was a problem hiding this comment.
Not that its super critical in this case, but when moving code, please move in a separate commit from rustfmt, such that git show --color-moved --color-moved-ws=ignore-space-change can highlight the move.
lightning/src/ln/channel.rs
Outdated
| remote_balance_before_fee_anchors_msat: u64, // remote balance before fees and anchors *not* considering dust limits | ||
| #[derive(Debug, PartialEq)] | ||
| pub(crate) struct CommitmentStats { | ||
| pub(crate) total_fee_sat: u64, // the total fee included in the transaction |
There was a problem hiding this comment.
Wanna go ahead and make the comments documentation?
lightning/src/ln/channel.rs
Outdated
| #[derive(Debug, PartialEq)] | ||
| pub(crate) struct CommitmentStats { | ||
| pub(crate) total_fee_sat: u64, // the total fee included in the transaction | ||
| pub(crate) local_balance_before_fee_msat: u64, // local balance before fees and anchors *not* considering dust limits |
There was a problem hiding this comment.
Comments are now wrong, as the fields are now post-anchors.
lightning/src/ln/channel.rs
Outdated
| @@ -4146,31 +4172,21 @@ where | |||
| { | |||
| let remote_commit_tx_fee_msat = if funding.is_outbound() { 0 } else { | |||
| let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered); | |||
| self.next_remote_commit_tx_fee_msat(funding, Some(htlc_candidate), None) // Don't include the extra fee spike buffer HTLC in calculations | |||
| self.next_remote_commit_tx_fee_msat(&funding, Some(htlc_candidate), None) // Don't include the extra fee spike buffer HTLC in calculations | |||
There was a problem hiding this comment.
Looks like unnecessary diff to add the & before funding here and below.
lightning/src/ln/channel.rs
Outdated
| // `Some(())` is for the fee spike buffer we keep for the remote. This deviates from | ||
| // the spec because the fee spike buffer requirement doesn't exist on the receiver's | ||
| // side, only on the sender's. Note that with anchor outputs we are no longer as | ||
| // sensitive to fee spikes, so we need to account for them. | ||
| // | ||
| // A `None` `HTLCCandidate` is used as in this case because we're already accounting for | ||
| // the incoming HTLC as it has been fully committed by both sides. | ||
| let mut remote_fee_cost_incl_stuck_buffer_msat = self.next_remote_commit_tx_fee_msat(funding, None, Some(())); | ||
| let mut remote_fee_cost_incl_stuck_buffer_msat = self.next_remote_commit_tx_fee_msat(&funding, None, Some(())); |
There was a problem hiding this comment.
Should we include the extra dust-buffer HTLC in the build_commitment_stats call, above, to match?
There was a problem hiding this comment.
Thank you for the suggestion. I looked into this, here's the major difference between the two paths:
next_remote_commit_tx_fee_msatwill set the dust threshold for HTLCs based oncontext.feerate_per_kw- the number of HTLCs passed to
build_commitment_statsabove come fromget_pending_htlc_stats, which itself will set the HTLC dust threshold according tocontext.get_dust_buffer_feerate(outbound_feerate_update)
Therefore number of nondust HTLCs will differ.
Therefore, commit_tx_fee_sat will differ.
lightning/src/ln/chan_utils.rs
Outdated
| cltv_expiry: cltv_expiry.0.unwrap(), | ||
| payment_hash: payment_hash.0.unwrap(), | ||
| transaction_output_index, | ||
| is_dust: transaction_output_index.is_none(), |
There was a problem hiding this comment.
Oh, should this just be an fn that checks if the output index is none? Would imply passing something other than HTLCOutputInCommitment to build_commitment_transaction (as at that point it doesn't have an output index), but IMO that's a cleaner API anyway.
Thanks I pinged Carla how does that sound ? |
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
5d6baeb to
3f98b95
Compare
lightning/src/ln/channel.rs
Outdated
| local_balance_before_fee_msat: u64, // local balance before fees and anchors *not* considering dust limits | ||
| remote_balance_before_fee_msat: u64, // remote balance before fees and anchors *not* considering dust limits |
There was a problem hiding this comment.
nit: comment remove "and anchors" from comments
lightning/src/sign/tx_builder.rs
Outdated
| value_to_self_after_htlcs: u64, value_to_remote_after_htlcs: u64, | ||
| channel_type: &ChannelTypeFeatures, | ||
| ) -> CommitmentStats { | ||
| let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, nondust_htlc_count, channel_type); |
There was a problem hiding this comment.
Would a goal be to allow implementations of TxBuilder to pick their own feerate (ie, not the value that commit_tx_fee_sat would set?
If yes, transaction_fee_satoshis would be wrong in get_available_balances. Doesn't look like this is actually used, but it would surface an incorrect value to end user.
There was a problem hiding this comment.
Thank you ! In that call, I am wondering whether we can subtract the value of the outputs from some channel_value_satoshis value and get the total fee that way... will think more this afternoon !
lightning/src/ln/channel.rs
Outdated
| total_fee_sat, | ||
| local_balance_before_fee_msat, | ||
| remote_balance_before_fee_msat: _, | ||
| } = SpecTxBuilder {}.build_commitment_stats(true, commitment_feerate, MIN_AFFORDABLE_HTLC_COUNT, value_to_self_msat, push_msat, &channel_type); |
There was a problem hiding this comment.
nit: here and a few others, move parameters onto newlines to save us needing to rustfmt them later
lightning/src/ln/channel.rs
Outdated
| if holder_balance_msat < buffer_fee_msat + funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { | ||
| let stats = self.build_commitment_stats(funding, true, true, Some(feerate_per_kw), Some(htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize)); | ||
| let holder_balance_msat = stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat; | ||
| // Note that `stats.total_fee_sat` now accounts for any HTLCs that transition from non-dust to dust under a higher feerate (in the case where HTLC-transactions pay endogenous fees). |
There was a problem hiding this comment.
iiuc previously we were using the old commitment feerate to build our commitment transaction (and thus possibly not trimming some HTLCs that became dust), and now we're using the new proposed feerate so we'll properly trim them to dust and account for them in our fees?
Would be nice to have a test before this commit that demonstrates the htcs not being properly trimmed, and then updated in this commit to show that they are.
lightning/src/ln/channel.rs
Outdated
| funding.get_channel_type(), | ||
| ); | ||
|
|
||
| // Note that this is now the capacity available after we have subtracted any non-zero-value anchors |
There was a problem hiding this comment.
(very) nitpicky: remove "this is now" from comment, here and above. Somebody reading this code from fresh won't be looking at the diff, so the comparison to the past state doesn't help them (commit message seems like a a better home for notes on what's changed).
There was a problem hiding this comment.
Certainly I tend to annotate my code for reviewers using comments like these :) Will move this to the git message thank you
lightning/src/ln/channel.rs
Outdated
| // We don't include AwaitingRemoteRevokeToRemove HTLCs because our next commitment | ||
| // transaction won't be generated until they send us their next RAA, which will mean | ||
| // dropping any HTLCs in this state. |
There was a problem hiding this comment.
nit: Move comment up to above the if-let? Bit out of place here
lightning/src/ln/channel.rs
Outdated
| let extra_htlc_commit_tx_fee_sat = TxBuilder::build_commitment_stats(&SpecTxBuilder {}, funding.is_outbound(), excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1 + on_counterparty_tx_offered_nondust_htlcs, 0, 0, funding.get_channel_type()).total_fee_sat; | ||
|
|
||
| let htlc_tx_fees_sat = chan_utils::htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs, on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type()); | ||
| let commit_tx_fee_sat = TxBuilder::build_commitment_stats(&SpecTxBuilder {}, funding.is_outbound(), excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + on_counterparty_tx_offered_nondust_htlcs, 0, 0, funding.get_channel_type()).total_fee_sat; |
There was a problem hiding this comment.
I do wonder whether we've got the right API here when we're making calls like this passing zero values for local/remote amounts (and passing a feerate that we don't need in another place).
Did you consider splitting some of the components of CommitmentStats out into their own methods?
There was a problem hiding this comment.
Yes I did, I agree with you I am not thrilled about this - we had a back and forth with Matt on this issue, will refresh on this this afternoon :)
| @@ -5102,9 +5132,15 @@ where | |||
| fn next_local_commit_tx_fee_msat( | |||
| &self, funding: &FundingScope, htlc: HTLCCandidate, fee_spike_buffer_htlc: Option<()>, | |||
| ) -> u64 { | |||
| let context = &self; | |||
| let context = self; | |||
There was a problem hiding this comment.
Let me see seems this removes the & ?
lightning/src/ln/channel.rs
Outdated
| funding.value_to_self_msat.saturating_sub(local_htlc_total_msat), | ||
| (funding.get_value_satoshis() * 1000).checked_sub(funding.value_to_self_msat).unwrap().saturating_sub(remote_htlc_total_msat), |
There was a problem hiding this comment.
Pull these out into their own vars here and in next_local_commit_tx_fee_msat to help readability a bit?
lightning/src/ln/channel.rs
Outdated
| let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len(); | ||
| let mut htlcs_included: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs); | ||
| let mut nondust_htlcs: Vec<HTLCOutputInCommitment> = Vec::with_capacity(num_htlcs); | ||
| let mut value_to_self_msat_offset = 0; |
There was a problem hiding this comment.
Non-blocking/personal preference: WDYT about tracking value_to_self_claimed and value_to_remote_claimed as separate u64 values and then used checked_sub below? For the sake of avoiding negative numbers / slight readability improvement.
There was a problem hiding this comment.
This is great thank you a prior goal of mine was to remove all the raw as casts here this one was the last one remaining !
…Builder` This is a temporary type that will be removed once we let `TxBuilder` choose which HTLCs are dust and which are non-dust.
In addition to the amount set by `commit_tx_fee_sat`, the transaction fee of a commitment transaction may also increase due to HTLC amounts that get burned to fees, and any elided anchors. We now include these amounts in the `transaction_fee_satoshis` field of `Balance::ClaimableOnChannelClose` too. This commit also makes the `transaction_fee_satoshis` field correct for custom transactions not encompassed in `chan_utils::commit_tx_fee_sat`.
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
3f98b95 to
a9892a9
Compare
There was a problem hiding this comment.
Major changes:
- Split out
build_commitment_stats, see 660b86e - Introduce
IncludedHTLCtype to enforce the dust status of a HTLC onTxBuilderwithout specifying the particular index, see f736d90- We will let
TxBuilderset this status in the next version of this API, for now we focus on its use in zero-fee commitments.
- We will let
Balance::ClaimableOnChannelClose.transaction_fee_satoshisnow includes the sum of the fee due to dust HTLCs, and non-dust HTLCs getting rounded down, see a9892a9- Introduce standalone commit to remove the last raw cast in
build_commitment_stats, see e26b232
Remaining TODOs:
- Append a commit to remove all
rustfmt::skipin the code we touched. - Write a test to demonstrate how we were previously not accounting for the decrease in weight due to newly trimmed HTLCs when checking whether we can afford a new feerate in
can_send_update_fee, see ebd94c2 - Proper git messages
| let extra_htlc_commit_tx_fee_sat = SpecTxBuilder {}.commit_tx_fee_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1 + on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type()); | ||
| let extra_htlc_htlc_tx_fees_sat = chan_utils::htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1, on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type()); |
There was a problem hiding this comment.
Question: do we expand the commit_tx_fee_sat call to also pass information back about the fees on HTLC transactions ?
… HTLCs when updating a feerate
|
🔔 1st Reminder Hey @carlaKC! This PR has been waiting for your review. |
hi @TheBlueMatt on this branch here i make an attempt at an API that supports "fixed additional outputs" on commitment transactions.
I place today's 330 sat anchors in that bucket, but the API should allow for custom ones.
The API should also let people customize all the existing outputs on the commitment transaction.
Still need to think about any remaining TODOs for additional outputs that are only present for some commitments (eg. DLC outputs), depending on how we'd like to prioritize these.